Add function to compute exposure cubes #398
Conversation
|
In order to make sure everything is working correctly I would be useful to add tests. https://github.com/tibaldo/gammapy/blob/master/gammapy/irf/tests/test_effective_area.py#L115 |
|
My 2 cents on tests: My experience is that high-level tests are more important than low-level unit tests, so if @tibaldo wants to prefers this now and then add a high-level test that computes an exposure cube and calls this with an array later, I'd say it's OK to merge this without extra test now. Of course having lots of unit tests is great, but it also slows people down, writing the tests becomes more work than implementing features. So bottom line: my suggestion for Gammapy would be to encourage / ask contributors to add high-level and unit tests, but it's also OK to say "no, I just want this feature to be merged" or "I'll add tests later". |
|
@cdeil: In principle I agree, but not in this case. An error at this point might be difficult to spot later and is very easy to avoid now, with a 3 line test that was probably made in an IPython notebook or similar anyways. An error at this point might also have implication for several high-level use cases. It also comes with the benefit that another Interpolator that might be added in the future is automatically tested. Of course I am not keeping anyone from merging their contributions if they want to, it was just a suggestion. |
| 'exposure_cube' | ||
| ] | ||
|
|
||
| log = logging.getLogger(__name__) |
There was a problem hiding this comment.
remove import logging and this line ... it's only needed from files where we do want to log, which is rare and I don't think needs to be added "just in case we want to emit log messages later"
|
I left a few nitpicky comments. From a quick look, the API and test and code looks good to me. @joleroi - Do you want to do any review / testing here or should we just merge (once the small issues are adressed) and continue testing / using this from master? |
|
No more comments from my side |
Exposure cube - abritrary ndarrays accepted as input for offset and energy in aeff interp
|
Don't forget to make an entry in https://github.com/gammapy/gammapy/blob/master/CHANGES.rst 😉 |
cdeil
changed the title
@joleroi please review, I implemented this as we discussed yesterday within the interpolation function. I will use this to implement 3D exposure cube, and we can merge once this is working